Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RHOAIENG-5489] Home page: projects w/ tiles #2741

Merged
merged 1 commit into from
Apr 24, 2024
Merged

[RHOAIENG-5489] Home page: projects w/ tiles #2741

merged 1 commit into from
Apr 24, 2024

Conversation

jeff-phillips-18
Copy link
Contributor

@jeff-phillips-18 jeff-phillips-18 commented Apr 23, 2024 •
edited by andrewballantyne
Loading

Closes: RHOAIENG-5489
Closes: RHOAIENG-5490
Closes: RHOAIENG-4860

Description

Add project tiles to the home page projects section.

How Has This Been Tested?

Locally tested while turning the home page feature floag on

Test Impact

Added e2e tests

Screen shots

Full tiles w/ self-provisioning access

image

Full tiles w/o self-provisioning access

image

Less tiles w/ self-provisioning access

image

Less tiles w/o self-provisioning access

image

Request review criteria:

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)
  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

/cc @jgiardino

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Apr 23, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 23, 2024

@jeff-phillips-18: GitHub didn't allow me to request PR reviews from the following users: jgiardino.

Note that only opendatahub-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Closes: RHOAIENG-5489

Description

Add project tiles to the home page projects section.

How Has This Been Tested?

Locally tested while turning the home page feature floag on

Test Impact

Added e2e tests

Screen shots

Full tiles w/ self-provisioning access

image

Full tiles w/o self-provisioning access

image

Less tiles w/ self-provisioning access

image

Less tiles w/o self-provisioning access

image

Request review criteria:

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)
  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

/cc @jgiardino

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jeff-phillips-18 jeff-phillips-18 force-pushed the home-page-projects branch 3 times, most recently from 93f397b to 8a0accb Compare April 23, 2024 15:35
@jeff-phillips-18 jeff-phillips-18 changed the title [WIP] Home page: projects w/ tiles [RHOAIENG-5489] Home page: projects w/ tiles Apr 23, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Apr 23, 2024
andrewballantyne
andrewballantyne requested changes Apr 23, 2024
View reviewed changes
frontend/src/pages/home/projects/ProjectCard.scss Outdated Show resolved Hide resolved
frontend/src/components/TruncatedText.tsx Outdated Show resolved Hide resolved
frontend/src/pages/home/projects/ProjectsSection.tsx Outdated Show resolved Hide resolved
@jeff-phillips-18 jeff-phillips-18 force-pushed the home-page-projects branch 5 times, most recently from da2d6c9 to 2ae6e4e Compare April 24, 2024 13:32
andrewballantyne
andrewballantyne reviewed Apr 24, 2024
View reviewed changes
frontend/src/components/TruncatedText.tsx
WebkitBoxOrient: 'vertical',
WebkitLineClamp: maxLines,
}}
{...rest}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting -- FWIW, if you provide style in rest you'll lose the primary purpose of the component

@openshift-ci openshift-ci bot added the lgtm label Apr 24, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 6391d08 into opendatahub-io:main Apr 24, 2024
6 checks passed
jgiardino
jgiardino reviewed Apr 24, 2024
View reviewed changes
frontend/src/pages/home/projects/CreateProjectCard.tsx
<>
<FlexItem>
<TextContent>
<Text component="h3">Need another project?</Text>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This font size looks really large relative to other contents on the page.
I'm also not seeing an h2 level heading after the h1 for the "Projects" section header.

image

One suggestion here is to use the h2 combined with a utility class for the font size:

<Text component="h3" class="pf-v5-u-font-size-sm" >Need another project?</Text>

When I look at the current design document, the sm size is used for this text.

@jgiardino
Copy link

Oh no! I just posted a comment and saw that this PR was merged. 😬
I was also going to say that these updates look really good. That one comment was the only thing I saw. But it's also really minor.

@andrewballantyne
Copy link
Member

@jgiardino if you get to it late you get to it late. @jeff-phillips-18 should still react to your feedback and he can sneak small things into future work or craft a new issue if it's larger. We don't usually wait on UX reviews as there will be more reviews later once the work is more complete.

@jeff-phillips-18 jeff-phillips-18 deleted the home-page-projects branch November 12, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgiardino jgiardino jgiardino left review comments

@andrewballantyne andrewballantyne andrewballantyne approved these changes

Assignees

@andrewballantyne andrewballantyne

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeff-phillips-18 @jgiardino @andrewballantyne